Add missing catalog tests #2955
Conversation
tests/integration/test_catalog.py
Outdated
| # list_namespaces returns a list of tuples | ||
| if isinstance(test_catalog, RestCatalog): | ||
| namespaces = test_catalog.list_namespaces() | ||
| assert ("new",) in namespaces or ("new.db",) in namespaces | ||
| else: | ||
| assert namespace in test_catalog.list_namespaces() |
There was a problem hiding this comment.
do you know why theres a behavior difference here?
ideally we're testing for consistent catalog behaviors in this class
There was a problem hiding this comment.
Calling list namespaces would get you:
rest_catalog - new
others - new.db
That's because iceberg-rest-fixture treats this as hierarchical namespaces, which many other catalogs don't support. The spec supports hierarchical namespaces, so that seems to be working as intended.
My comment was very poor, so I wrote an actual comment to explain this.
There was a problem hiding this comment.
We should tag the different catalogs with different feature sets (hierarchical namespaces, supports slashes, etc.) and then have the catalog tests base their behavior based off those tags
|
Nice! |
Fokko
left a comment
Fokko
left a comment
There was a problem hiding this comment.
Thanks @rambleraptor 🙌
I've merged #2972 Can you resolve the conflicts here?
rambleraptor
force-pushed
the
catalog_tests3
branch
from
89d316a to
4ed9cf8
Compare
|
@Fokko resolved the conflicts and changed the tests to use the new |
|
Thanks @rambleraptor, I'll leave the PR open for @kevinjqliu to merge since there are some open comments :) |
| if isinstance(test_catalog, HiveCatalog): | ||
| pytest.skip("HiveCatalog raises NoSuchObjectException instead of NoSuchNamespaceError") |
There was a problem hiding this comment.
we should probably change HiveCatalog then 😄
4 participants
Rationale for this change
There's a couple missing catalog tests around supporting namespaces + tables with slashes/dots. Along the way, I found an issue in how we create the CreateTableRequest + RegisterTableRequest
Are these changes tested?
Tests are included.
Are there any user-facing changes?